Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Reverse ordering support for Task API #816

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ma1581
Copy link
Contributor

@ma1581 ma1581 commented Feb 9, 2025

Pull Request

Related issue

Fixes #798

What does this PR do?

  • Accepts 'reverse' field to provider ordering flexibility

PR checklist

Please check if your PR fulfills the following requirements:

  • [ Y ] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • [ Y ] Have you read the contributing guidelines?
  • [ Y ] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@ma1581 ma1581 marked this pull request as ready for review February 9, 2025 16:09
@Strift
Copy link
Contributor

Strift commented Feb 17, 2025

Hi @ma1581, thank you for your PR. 🙌

I merged the changes from the main branch (related to your other PR.) But now, the tests are not passing. Can you look into it?

Thanks!

@ma1581
Copy link
Contributor Author

ma1581 commented Feb 22, 2025

Hi @Strift . thanks for notifying this.
Have updated the implementation and junit to match the time format. pls check it

Glad to be part of this

@Strift Strift requested a review from curquiza February 27, 2025 08:17
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your PR!

Next time I would ask you to make one change per PR, it makes it easier to review :)

Comment on lines +18 to +22
@BeforeEach
void beforeEach() {
TimeZone.setDefault(TimeZone.getTimeZone("UTC"));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you need to set this? And if "yes" why not to set this for the whole test suite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary entirely, as it works find even without it. but when running the test cases locally test cases related to dates, they are bound to fail.
this will allow devs to be sure that no False negative happens when testing locally.

if my above reasoning is fine, will apply to this to entire test suite in a new commit

Comment on lines 221 to 225
Task[] defaultTaskList =
client.getTasks(new TasksQuery().setAfterEnqueuedAt(currentTime)).getResults();
Task[] reversedTaskList =
client.getTasks(new TasksQuery().setAfterEnqueuedAt(currentTime).setReverse(true))
.getResults();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you map only the task ids here, and then you assert them later, so the test will be less complex to read.

Arrays.stream(defaultTaskList).map(Task::getUid).collect(Collectors.toList());
List<Integer> reversedTaskOrder =
Arrays.stream(reversedTaskList).map(Task::getUid).collect(Collectors.toList());
assertFalse(originalTaskOrder.isEmpty());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also remove one of these assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed assertFalse(reversedTaskOrder.isEmpty()

@ma1581
Copy link
Contributor Author

ma1581 commented Mar 8, 2025

Hi @brunoocasali .
have resolved the comments , added comments for rest. Please take a look into it
Thanks

@ma1581 ma1581 requested a review from brunoocasali March 8, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.12.0] Add reverse parameter for fetching tasks
3 participants